Repository navigation
Conversation
CatarinaGamboa
left a comment
CatarinaGamboa
left a comment
There was a problem hiding this comment.
Three things, mostly about what happens when something goes wrong.
Reviewed with Claude Code (reviewer + adversarial agents per PR, findings checked against the code before posting).
| const nextFixtureDiagnostics = () => new Promise<LJDiagnostic[]>((resolve) => { | ||
| const subscription = api.onDiagnostics((diagnostics) => { | ||
| if (diagnostics.some(d => d.type === 'refinement-error' && path.resolve(d.file) === uri.fsPath)) { | ||
| const matches = passing ? diagnostics.length === 0 |
There was a problem hiding this comment.
A regression shows up as a bare 120s timeout. This wait only resolves when the result is the expected one (empty for passing, a refinement error for failing). If the passing fixture starts getting an error, or the failing one gets none, the promise never resolves. The asserts below (including assert.deepEqual(diagnostics, []) on line 46, which can't fail) are never reached, and the failure is a timeout with no diagnostics in the output.
Suggest resolving on the first diagnostics notification for this run and asserting on it, so a failure prints what actually came back.
There was a problem hiding this comment.
Fixed in e5b2580: waits capture the first diagnostics notification and assert its contents; crashes fail immediately with fixture/status details. Both fixtures passed on stable and minimum VS Code, and a focused harness verified unexpected results fail promptly.
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| version: ${{ (github.event_name == 'pull_request' || github.ref == 'refs/heads/main') && fromJSON('["stable", "minimum"]') || fromJSON('["stable"]') }} |
There was a problem hiding this comment.
Two things with this job:
- This PR removes the fork-only
ifthat Add real VS Code integration smoke test #144 had onintegration. So for same-repo PRs every push runs it twice: the push run (stable), plus the PR run (stable + minimum). - Release tags only test
stable.publish.ymlcalls this workflow with eventpushand refrefs/tags/v*, so neither branch of this condition matches. Releases then never test the minimum supported VS Code version.
Suggest bringing the if back, but still letting minimum run for PRs and tags. For example, put the decision in the matrix and the if, so that pushes to branches run stable, while pushes to main, tags (startsWith(github.ref, 'refs/tags/')) and fork PRs run both.
There was a problem hiding this comment.
Fixed in e5b2580: integration skips duplicate same-repository PR runs; main, release tags, and fork PRs test stable plus minimum. Required Checks remains unconditional.
…odex/issue-132-passing-min-vscode
Co-authored-by: Codex <noreply@openai.com>
…nto codex/issue-132-passing-min-vscode
CatarinaGamboa
left a comment
CatarinaGamboa
left a comment
There was a problem hiding this comment.
The minimum (1.82.0) leg never runs for same-repo PRs: the matrix only adds it for pull_request/main/tags, but the job if: skips same-repo pull_request events, and branch pushes only get stable. E.g. latest runs on #145/#146/#147 only show "VS Code integration (stable)". Maybe include minimum on push events too?
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
|
Addressed the missing minimum-version coverage: integration now uses a static Independent review, workflow checks, extension installation, and both integration legs in the final CI run passed. |
Adds real VS Code coverage for webview readiness, diagnostics/context messages, and Stop, Start, and Restart. Checks process termination and verification after Restart, and fixes a shutdown race that could clear the newly started server. Validated both fixtures locally and in CI on stable and VS Code 1.82.0; lint, types, and installation passed. Depends on #145. Closes #133. Generated by Codex. --------- Co-authored-by: Codex <noreply@openai.com>
## Description Closes #134. Add weekly and manual Windows/macOS runs for client and server unit tests plus VS Code stable integration tests, with logs uploaded on failure. The diff contains only `platform-tests.yml`. ## Related Issues Depends on #146. The existing prerequisite PRs now form one chain through #145, #144, #143, #141, #142, #140, #139, #138, and #137 to main. The schedule becomes active when merged to the default branch. Validation: 32 client tests, 24 server tests, lint, production/test TypeScript checks, and extension installation passed. Stable and minimum VS Code integration passed in [PR CI](https://github.com/liquid-java/vscode-liquidjava/actions/runs/37541530280). Fresh [Windows/macOS validation](https://github.com/liquid-java/vscode-liquidjava/actions/runs/37541530341) passed on the final commit, including both unit-test suites and VS Code integration. 🤖 Generated with [Codex](https://openai.com/codex/) --------- Co-authored-by: Codex <noreply@openai.com>
3 participants
Add an isolated passing workspace and assert the first diagnostic result after Verify. Run both passing and failing fixtures on stable and the minimum supported VS Code (1.82.0) for every pull request, main, and reusable release check.
Validated fixture failure reporting, workflow coverage, lint, types, and extension installation.
Depends on #144. Closes #132.
🤖 Generated with Codex